Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix for the terminal shutdown issue #4180

Merged
merged 1 commit into from
Jun 7, 2020
Merged

Fix for the terminal shutdown issue #4180

merged 1 commit into from
Jun 7, 2020

Conversation

gdlmx
Copy link
Contributor

@gdlmx gdlmx commented Nov 7, 2018

This PR is aimed to fix the problem of Zombie terminal as reported in issue 5061 of JupyterLab. It adds checking for the parameter of websocket request against existing terminals in order to avoid unexpected creation of terminals. According to the issue report, the xtermjs client tries to recreate the websocket connection after termination of the shell process, which then triggers the creation of another terminal (shell process) on the server.

The issue can also be "fixed" in the upstream project terminado, in which the default behavior of NamedTermManager is to automatically start a new terminal in response to a websocket handshake. However, the fix proposed in this PR is cleaner and more efficient.

This patch prevents creation of a new terminal when handling websocket handshaking request. The default behavior of `terminado.NamedTermManager` is to automatically start a new terminal in response to a websocket handshake, which prevents a terminal from being properly shut down in JupyterLab as reported in this [issue](jupyterlab/jupyterlab#5061).
@takluyver
Copy link
Member

I consider it a nice hidden feature that you can create named terminals just by going to the URL, so it would be a shame to lose it. Do you know if there's an issue open about it on xtermjs? I think it should be technically possible for the server side to close down the connection and indicate to xtermjs that it's deliberately being closed.

@gdlmx
Copy link
Contributor Author

gdlmx commented Nov 16, 2018

@takluyver I didn't find a relevant issue on xtermjs. In xtermjs, the onclose handler doesn't check the websocket CloseEvent at all, probably because the property values of this event may be browser specific:

I can get "wasClean == true" if I let the client initiate the close() and the server finish with a close() (which is spec). I get a "true" on chrome but not on firefox if the server initiates the close()

Implementing such a feature in xtermjs will require lots of testing.

@gdlmx
Copy link
Contributor Author

gdlmx commented Nov 16, 2018

I consider it a nice hidden feature that you can create named terminals just by going to the URL

I agree if the creation is triggered by a http GET request, not by a websocket handshake. Usually the websocket is managed automatically in the background which the user has no direct control on.

@blink1073
Copy link
Contributor

I agree with this sentiment: starting a terminal should be an HTTP request, not a websocket request.

Copy link
Contributor

@blink1073 blink1073 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@blink1073 blink1073 added this to the 6.1 milestone Jun 7, 2020
@blink1073 blink1073 merged commit 866f4f8 into jupyter:master Jun 7, 2020
@rgbkrk
Copy link
Member

rgbkrk commented Aug 21, 2020

What would folks think about a special named path like /terminals/new that would create a new terminal?

@blink1073
Copy link
Contributor

That seems reasonable to be, but only if it redirected you to a new numbered terminal url.

@Zsailer
Copy link
Member

Zsailer commented Aug 21, 2020

Seems reasonable to me too. Maybe add to jupyter_server as well 😉

@kevin-bates
Copy link
Member

I feel like I'm missing some context. How is this different than a POST against /api/terminals? Are you talking about bringing your own terminal names (rather than numbers) this way? Could someone elaborate a bit more on this? Thanks.

@blink1073
Copy link
Contributor

This would be a URL you visit in your browser, a GET that gives a 302 to a named terminal URL.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants